Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

core: support range for random selection #1783

Merged
merged 10 commits into from
Oct 21, 2019
Merged

core: support range for random selection #1783

merged 10 commits into from
Oct 21, 2019

Conversation

rleungx
Copy link
Member

@rleungx rleungx commented Sep 29, 2019

What problem does this PR solve?

This PR is mainly for supporting range for schedulers.

What is changed and how it works?

This PR changes the region map to region tree and supports random selection according to a given range.

Check List

Tests

  • Unit test

Side effects

  • Possible performance regression

@rleungx rleungx added the component/schedule Scheduling logic. label Sep 29, 2019
@nolouch nolouch added the type/enhancement The issue or PR belongs to an enhancement. label Sep 29, 2019
@codecov-io
Copy link

codecov-io commented Sep 30, 2019 •
edited
Loading

Codecov Report

Merging #1783 into master will decrease coverage by 0.15%.
The diff coverage is 87.28%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1783      +/-   ##
==========================================
- Coverage    78.1%   77.95%   -0.16%     
==========================================
  Files         167      167              
  Lines       16851    16907      +56     
==========================================
+ Hits        13162    13180      +18     
- Misses       2613     2642      +29     
- Partials     1076     1085       +9
Impacted Files Coverage Δ
server/core/basic_cluster.go 95% <100%> (ø) ⬆️
server/core/storage.go 76.31% <100%> (ø) ⬆️
server/cluster.go 84.91% <50%> (ø) ⬆️
server/core/region_storage.go 81.81% <50%> (ø) ⬆️
server/core/region_tree.go 93.57% <83.72%> (-4.18%) ⬇️
server/core/region.go 91.73% <90.1%> (-0.96%) ⬇️
server/core/test_util.go 96.2% <92.85%> (+0.09%) ⬆️
pkg/metricutil/metricutil.go 90.62% <0%> (-9.38%) ⬇️
server/kv/etcd_kv.go 72.72% <0%> (-9.1%) ⬇️
server/schedulers/shuffle_hot_region.go 56.17% <0%> (-5.62%) ⬇️
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c456deb...993cd76. Read the comment docs.

@rleungx
Copy link
Member Author

rleungx commented Sep 30, 2019 

master:
BenchmarkRandomRegion-40    	50000000	        37.0 ns/op	       0 B/op	       0 allocs/op
PASS

tree:
BenchmarkRandomRegion-40    	 1000000	      1688 ns/op	       0 B/op	       0 allocs/op
PASS

@rleungx rleungx force-pushed the tree branch 4 times, most recently from 757e9eb to dd25be6 Compare October 9, 2019 05:44
@rleungx
Copy link
Member Author

rleungx commented Oct 9, 2019 

master:
BenchmarkAddRegion-40    	  500000	      4909 ns/op	    1120 B/op	      21 allocs/op

tree:
BenchmarkAddRegion-40    	  300000	      5662 ns/op	    1797 B/op	      45 allocs/op

nolouch
nolouch reviewed Oct 10, 2019
View reviewed changes
server/core/region.go Outdated Show resolved Hide resolved
Luffbee
Luffbee suggested changes Oct 10, 2019
View reviewed changes
server/core/region_tree.go Outdated Show resolved Hide resolved
server/core/region_tree.go Outdated Show resolved Hide resolved
server/core/region_tree.go Outdated Show resolved Hide resolved
server/core/region_tree.go Outdated Show resolved Hide resolved
server/core/region_tree.go Outdated Show resolved Hide resolved
server/core/region_tree.go Show resolved Hide resolved
server/core/region_tree.go Outdated Show resolved Hide resolved
Luffbee
Luffbee reviewed Oct 10, 2019
View reviewed changes
server/core/region_tree.go Outdated Show resolved Hide resolved
server/core/region_tree.go Outdated Show resolved Hide resolved
@rleungx rleungx mentioned this pull request Oct 10, 2019
Merged
2 tasks
shafreeck
shafreeck reviewed Oct 10, 2019
View reviewed changes
server/core/region.go Show resolved Hide resolved
server/core/region.go
@@ -460,25 +452,95 @@ func (rm *regionMap) TotalSize() int64 {
return rm.totalSize
}

// regionSubTree is used to manager different types of regions.
type regionSubTree struct {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name regionSubTree and regionTree seems confused. Maybe we should name it according to the purpose?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but I cannot find a proper name.

server/core/region.go Outdated Show resolved Hide resolved
Luffbee
Luffbee suggested changes Oct 15, 2019
View reviewed changes
server/core/region_tree.go Outdated Show resolved Hide resolved
server/core/region_tree.go Outdated Show resolved Hide resolved
rleungx
rleungx commented Oct 15, 2019
View reviewed changes
server/core/region_tree.go Outdated Show resolved Hide resolved
Luffbee
Luffbee reviewed Oct 15, 2019
View reviewed changes
server/core/region_tree_test.go Show resolved Hide resolved
server/core/region_tree.go Show resolved Hide resolved
server/core/region.go Outdated Show resolved Hide resolved
@nolouch nolouch added the priority/P1 The issue has P1 priority. label Oct 15, 2019
@rleungx rleungx requested a review from Luffbee October 16, 2019 05:41
nolouch
nolouch approved these changes Oct 17, 2019
View reviewed changes
Copy link
Contributor

@nolouch nolouch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. PTAL @Luffbee

Luffbee
Luffbee reviewed Oct 18, 2019
View reviewed changes
server/core/region.go Outdated Show resolved Hide resolved
server/core/region_tree_test.go Outdated Show resolved Hide resolved
@rleungx
replace meta with RegionInfo
338c051
@rleungx
support range in region tree
82ddb93 
Signed-off-by: Ryan Leung <rleungx@gmail.com>
@rleungx
add random range benchmark
2ef94a6
@rleungx
add insert region benchmark
d6f199c 
Signed-off-by: Ryan Leung <rleungx@gmail.com>
@rleungx
address the comments
2036c5e 
Signed-off-by: Ryan Leung <rleungx@gmail.com>
@rleungx
address the comment
27a1137 
Signed-off-by: Ryan Leung <rleungx@gmail.com>
@rleungx
address the comments
ac30880 
Signed-off-by: Ryan Leung <rleungx@gmail.com>
@rleungx
address the comment
d03fdfc 
Signed-off-by: Ryan Leung <rleungx@gmail.com>
@rleungx
address the comment
9d425dd 
Signed-off-by: Ryan Leung <rleungx@gmail.com>
Luffbee
Luffbee approved these changes Oct 21, 2019
View reviewed changes
Copy link
Contributor

@Luffbee Luffbee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@nolouch nolouch added the status/can-merge Indicates a PR has been approved by a committer. label Oct 21, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Oct 21, 2019

/run-all-tests

@sre-bot sre-bot merged commit 752ecd7 into tikv:master Oct 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shafreeck shafreeck shafreeck left review comments

@Luffbee Luffbee Luffbee approved these changes

@nolouch nolouch nolouch approved these changes

@lhy1024 lhy1024 Awaiting requested review from lhy1024

Assignees
No one assigned
Labels
component/schedule Scheduling logic. priority/P1 The issue has P1 priority. status/can-merge Indicates a PR has been approved by a committer. type/enhancement The issue or PR belongs to an enhancement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@rleungx @codecov-io @sre-bot @shafreeck @nolouch @Luffbee